Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pre commit python #11004

Merged
merged 10 commits into from
Nov 14, 2022
Merged

Pre commit python #11004

merged 10 commits into from
Nov 14, 2022

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 25, 2022 •
edited
Loading

It turns out the "language: python" are working only by luck.
A python hook requires a python entry. That ensures that the same python version is used and not another one found at the path.

Our use case of starting a system script is covered by "language: system"

This also pins the clang-format version and gets around the clang-format not found issue.

@daschuer daschuer mentioned this pull request Oct 25, 2022
Closed
@daschuer
Copy link
Member Author

@JoergAtGithub does this work for you on windows?

@JoergAtGithub
Copy link
Member

All cheks make Pass on Windows with this PR, but it seems, that clang-format doesn't detect or correct anything.

@daschuer
Copy link
Member Author

Did you to artificially introduce a formatting issue?
I got:

clang-format.............................................................Failed
- hook id: clang-format
- files were modified by this hook

[INFO] First pass: Reformatting added/changed lines...
[INFO] Reformatting src/main.cpp [46-46, 66-66, 80-83]...
[INFO] Second pass: Breaking long added/changed lines...
[INFO] Reformatting src/main.cpp [46-46, 66-66]...

I also noticed that since
f1dce76
we no longer have a length check for comments.

@JoergAtGithub
Copy link
Member

After signifiantly destroying the formatting of a .cpp file I get this:

fix utf-8 byte order marker..............................................Passed
check for case conflicts.................................................Passed
check json...........................................(no files to check)Skipped
check for merge conflicts................................................Passed
check xml............................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.............................(no files to check)Skipped
don't commit to branch...................................................Passed
codespell................................................................Passed
eslint...............................................(no files to check)Skipped
clang-format.............................................................Passed
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
shellcheck...........................................(no files to check)Skipped
markdownlint-cli2....................................(no files to check)Skipped
Validate GitHub Workflows............................(no files to check)Skipped
prettier.............................................(no files to check)Skipped
qsscheck.............................................(no files to check)Skipped
changelog............................................(no files to check)Skipped
metainfo.............................................(no files to check)Skipped
[pr/11004 232c856183] Testcommit
 1 file changed, 3 insertions(+), 8 deletions(-)

and the .cpp file is commited unmodified.

@daschuer daschuer force-pushed the pre-commit-python branch 2 times, most recently from 5d4b5a2 to 006f24a Compare October 27, 2022 05:36
@daschuer
Copy link
Member Author

How did you run pre-commit?
Does it work during an actual commit?

@daschuer
Copy link
Member Author

Ah, it was a separator issue with / and
@JoergAtGithub can you please retry.

@daschuer
Copy link
Member Author

I have added some other couple of windows fixes. I think you can now use pre-commit on windows.
Please confirm.

@daschuer daschuer mentioned this pull request Oct 28, 2022
Merged
@JoergAtGithub
Copy link
Member

Now it works for me!

@daschuer
Copy link
Member Author

Cool, thank you. Can you also confirm that pre-commit works now without disabling any check?

@JoergAtGithub
Copy link
Member

Yes, nothing is disbabled!

@daschuer
Copy link
Member Author

Thank you for confirming. Who can press merge?
Note, that this PR just fixes the windows issue without changing any general workflow as proposed in #10972.

@ywwg
Copy link
Member

ywwg commented Nov 3, 2022

This still fails on Ubuntu 22.04 but that version of python is known broken:

An unexpected error has occurred: AssertionError: BUG: expected environment for python to be healthy immediately after install, please open an issue describing your environment

more info:

virtualenv python version did not match created version:
- actual version: <<error retrieving version from /home/owen/.cache/pre-commit/repoujfceyzn/py_env-python3/bin/python>>
- expected version: 3.10.6.final.0

Check the log at /home/owen/.cache/pre-commit/pre-commit.log

using python 3.11, which has been my workaround, this works fine still

@daschuer
Copy link
Member Author

daschuer commented Nov 3, 2022 •
edited
Loading

Ok, it looks like the pre-commit issue is tracked here:
pre-commit/pre-commit#2299

The workaround using the python3.11 package has been confirmed or the patched python3.10 found here:
https://launchpad.net/~deadsnakes/+archive/ubuntu/python3.10-jammy

This is the Ubuntu bug:
https://bugs.launchpad.net/ubuntu/+source/python3.10/+bug/1967920

@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2022

I have updated the wiki accordingly:
https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2022

Ready for merge!

@daschuer daschuer requested a review from Holzhaus November 7, 2022 19:35
@ronso0
Copy link
Member

ronso0 commented Nov 14, 2022

Since no one familiar wit pre-commit finds time to take a look, I'll press merge now to move forward. Regressions we be fixed.
Changes seem reasonable from what I can tell and CI passes, though I'm curious how ReflowComments: false works out.

@ronso0 ronso0 merged commit 7335ad5 into mixxxdj:2.3 Nov 14, 2022
@daschuer daschuer deleted the pre-commit-python branch November 16, 2022 08:22
@daschuer daschuer mentioned this pull request Dec 21, 2022
Closed
@JoergAtGithub JoergAtGithub mentioned this pull request Dec 23, 2022
Merged
@JoergAtGithub JoergAtGithub mentioned this pull request Jan 15, 2023
Merged
@daschuer daschuer mentioned this pull request Mar 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Awaiting requested review from Holzhaus

Assignees
No one assigned
Labels
code quality needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daschuer @JoergAtGithub @ywwg @ronso0